Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[YUNIKORN-2817] Pods should be deleted immediately when they are plac… #896

Closed
wants to merge 3 commits into from

Conversation

zhuqi-lucas
Copy link
Contributor

@zhuqi-lucas zhuqi-lucas commented Aug 18, 2024

…eholder pods

What is this PR for?

Pods should be deleted immediately when they are placeholder pods, for example when placeholder timeout, we clean up them and the shim side should finally delete those pods without delay.

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

Todos

  • - Task

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

  • - The licenses files need update.
  • - There is breaking changes for older versions.
  • - It needs documentation.

Copy link

codecov bot commented Aug 18, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 67.85%. Comparing base (2278b32) to head (609692c).
Report is 3 commits behind head on master.

Files Patch % Lines
pkg/client/kubeclient.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #896      +/-   ##
==========================================
+ Coverage   67.84%   67.85%   +0.01%     
==========================================
  Files          70       70              
  Lines        7485     7482       -3     
==========================================
- Hits         5078     5077       -1     
+ Misses       2195     2194       -1     
+ Partials      212      211       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@craigcondit craigcondit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unnecessary. We already create the placeholder pods with the timeout set to 0. There's no need to do it again here.

@zhuqi-lucas
Copy link
Contributor Author

zhuqi-lucas commented Aug 18, 2024

Thanks @craigcondit for review, i think we need to change both setting for pod spec and also the actually deleting settings, and the actually deleting settings will be force executed. There are two timeout for pod, when we create the placeholder pod we set the pod spec timeout to zero now, but when we actually delete the pod, the timeout will be potentially force override by the for API timeout setting:

For creating placeholder, we setting in this code:

TerminationGracePeriodSeconds: &zeroSeconds,

// Optional duration in seconds the pod needs to terminate gracefully. May be decreased in delete request.
	// Value must be non-negative integer. The value zero indicates stop immediately via
	// the kill signal (no opportunity to shut down).
	// If this value is nil, the default grace period will be used instead.
	// The grace period is the duration in seconds after the processes running in the pod are sent
	// a termination signal and the time when the processes are forcibly halted with a kill signal.
	// Set this value longer than the expected cleanup time for your process.
	// Defaults to 30 seconds.
	// +optional
	TerminationGracePeriodSeconds *int64 `json:"terminationGracePeriodSeconds,omitempty" protobuf:"varint,4,opt,name=terminationGracePeriodSeconds"`

Actually pod deletion override code, we will finally call this for all pod deletion, we currently force setting to 3 seconds for all pods deletion:

GracePeriodSeconds: &gracefulSeconds,

// The duration in seconds before the object should be deleted. Value must be non-negative integer.
	// The value zero indicates delete immediately. If this value is nil, the default grace period for the
	// specified type will be used.
	// Defaults to a per object value if not specified. zero means delete immediately.
	// +optional
	GracePeriodSeconds *int64 `json:"gracePeriodSeconds,omitempty" protobuf:"varint,1,opt,name=gracePeriodSeconds"`

And this is the link documented the force override for API pod deletion:

https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#pod-termination-forced

@craigcondit
Copy link
Contributor

I wasn't aware we were setting 3 seconds currently. This is wrong; we shouldn't be overriding pod settings in deletion like that. The real fix here is to remove the 3 second override. Imagine if we delete a non-placeholder pod such as a database: Forcing non-graceful termination after 3 seconds could lead to data corruption. The override has no place in the generic Delete() call.

@craigcondit
Copy link
Contributor

This could also explain some performance issues seen in gang scheduling. If all placeholders are taking 3 seconds to be deleted that adds significant delays to gang scheduling jobs.

@zhuqi-lucas
Copy link
Contributor Author

zhuqi-lucas commented Aug 18, 2024

This could also explain some performance issues seen in gang scheduling. If all placeholders are taking 3 seconds to be deleted that adds significant delays to gang scheduling jobs.

Yeah, i agree, and i am in favor of removing this override, it's more reasonable instead of adding new zero override. And final question for non placeholder pod, we default 3 seconds here, do we also need to remove it, or we only remove the placeholder pods override? If we do not override the non-placeholder pods, it's default timeout should be 30s.

@craigcondit
Copy link
Contributor

When deleting, the default is whatever the pod specified and 30 seconds otherwise. We shouldn't change that logic; just remove the 3s override we have now.

@zhuqi-lucas
Copy link
Contributor Author

Thanks @craigcondit , reasonable suggestion and addressed in latest PR.

Copy link
Contributor

@craigcondit craigcondit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants